Improve evaluate-pr-tests workflow: pull_request_target, access gating, security docs#34678
Improve evaluate-pr-tests workflow: pull_request_target, access gating, security docs#34678
Conversation
- Change trigger from pull_request to pull_request_target so fork PRs have access to secrets (COPILOT_GITHUB_TOKEN) - Add roles: all to allow fork contributors (who have read permission) to trigger the workflow - Remove forks: ["*"] (not needed with pull_request_target) - Remove ready_for_review type (not supported by gh-aw for pull_request_target) - Update if condition and gate step to reference pull_request_target Validated on PureWeen/maui: - Same-repo PR: all green (run 23603776593) - Fork PR via workflow_dispatch: all green (run 23605610535) - Fork PR via pull_request_target: all green (run 23606033617) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 34678Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 34678" |
There was a problem hiding this comment.
Pull request overview
Updates the gh-aw Evaluate PR Tests workflow triggers so fork PRs can be evaluated (by switching from pull_request to pull_request_target) while keeping the workflow’s gating/conditions aligned with the new event.
Changes:
- Switched workflow trigger from
pull_requesttopull_request_targetand updated relatedif:conditions. - Updated the gate step condition to run under
pull_request_target. - Updated the compiled
.lock.ymlto reflect the new trigger configuration.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
.github/workflows/copilot-evaluate-tests.md |
Moves evaluation to pull_request_target and updates workflow conditions/gate accordingly. |
.github/workflows/copilot-evaluate-tests.lock.yml |
Regenerated compiled workflow reflecting the trigger/condition changes. |
| roles: all | ||
|
|
There was a problem hiding this comment.
roles: all is indented under on: (same level as workflow_dispatch), so the gh-aw compiler treats it as part of the event config instead of a top-level workflow setting. In the compiled lock file this ends up commented out and the membership/role gate is effectively removed, meaning anyone (including arbitrary /evaluate-tests issue comments) can trigger a pull_request_target run that has access to repository secrets. Unindent roles: all to the top level (aligned with on:/if:) and re-run gh aw compile so the intended role gating is applied.
| roles: all | |
| roles: all |
There was a problem hiding this comment.
This suggestion is incorrect. The gh aw compile command explicitly requires roles under on: — placing it at the top level produces:
error: Unknown property: roles. Did you mean: bots, jobs, tools. 'roles' belongs under 'on'
The lock file comment # roles: all # Roles processed as role check in pre-activation job is the compiler's standard format. With roles: all, the pre_activation role check is intentionally omitted (everyone is allowed). This was validated on PureWeen/maui where a fork contributor with read permission successfully triggered the workflow (run 23606033617).
There was a problem hiding this comment.
@PureWeen's explanation is correct. The reviewer's suggestion has two issues:
-
roles: allplacement: The gh-aw compiler requiresrolesunderon:— placing it at the top level produces a compile error. The lock file line# roles: all # Roles processed as role check in pre-activation jobis the compiler's standard format confirming the setting was processed. Withroles: all, the compiler intentionally skips the pre-activation role gate (because everyone is allowed), which is exactly the intended behavior for fork contributors. -
The fork guard suggestion (
head.repo.fork == falsein the activationif:): This would completely defeat the purpose of this PR, which is specifically to allow fork PRs to be evaluated.
The security model is sound:
pull_request_targetruns the workflow from the base branch — forks cannot alter it- The agent container has all credentials scrubbed
safe-outputslimits the agent to posting 1 comment- The workflow is read-only (evaluate tests, post a comment)
No changes needed here.
| pull_request_target: | ||
| paths: | ||
| - src/**/tests/** | ||
| - src/**/test/** | ||
| types: | ||
| - opened | ||
| - synchronize | ||
| - reopened | ||
| - ready_for_review | ||
| # roles: all # Roles processed as role check in pre-activation job | ||
| workflow_dispatch: |
There was a problem hiding this comment.
The compiled lock file shows roles: all commented out under the on: block, which indicates the roles setting was not applied. This also coincides with the removal of the pre_activation/membership check job, so the workflow no longer gates who can run it. Fix by moving roles: to the correct top-level location in the .md frontmatter and re-compiling so the lock file includes the intended role check behavior.
| needs: pre_activation | ||
| if: > | ||
| (needs.pre_activation.outputs.activated == 'true') && ((github.event_name == 'pull_request' && github.event.pull_request.draft == false) || github.event_name == 'workflow_dispatch' || (github.event_name == 'issue_comment' && | ||
| (github.event_name == 'pull_request_target' && github.event.pull_request.draft == false) || github.event_name == 'workflow_dispatch' || (github.event_name == 'issue_comment' && |
There was a problem hiding this comment.
jobs.activation no longer depends on a role/membership gate (needs: pre_activation and the needs.pre_activation.outputs.activated check are gone). With pull_request_target, this means the workflow can run with secrets for any matching PR/comment, which is a significant security/cost exposure. After fixing the roles placement in the .md, ensure the compiled lock restores the gating (or add an explicit guard) before allowing activation to proceed.
| (github.event_name == 'pull_request_target' && github.event.pull_request.draft == false) || github.event_name == 'workflow_dispatch' || (github.event_name == 'issue_comment' && | |
| (github.event_name == 'pull_request_target' && github.event.pull_request.draft == false && github.event.pull_request.head.repo.fork == false) || github.event_name == 'workflow_dispatch' || (github.event_name == 'issue_comment' && |
The workflow_dispatch step runs with GITHUB_TOKEN and checks out PR code. Restrict it to only process PRs from authors with write/maintain/admin access, preventing checkout of untrusted fork code in a privileged context. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move the PR author permission check from inline workflow bash into the shared Checkout-GhAwPr.ps1 script. Any gh-aw workflow using this script now automatically gates on the PR author having write/maintain/admin access before checking out code. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fork PRs are handled by pull_request_target (platform checkout in sandboxed container). The workflow_dispatch path should only process same-repo PRs from authors with write access. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Restoring only skills/, instructions/, and copilot-instructions.md left other .github/ subdirs (pr-review/, scripts/, workflows/) from the PR branch. Restore the entire .github/ directory for complete coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Instead of deleting .github/ and restoring from main, merge the base branch into the PR branch after checkout. This produces the same state as a pull_request merge commit: PR changes + latest main. If the PR modifies a skill, the PR version wins; otherwise main's version is used. This lets contributors iterate on skills via workflow_dispatch while keeping everything else current. On merge conflict, falls back to the PR branch as-is with a warning. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- pull_request_target: only auto-runs for OWNER/MEMBER/COLLABORATOR - issue_comment: /evaluate-tests only accepted from OWNER/MEMBER/COLLABORATOR - workflow_dispatch: unchanged - External PRs require maintainer /evaluate-tests comment to trigger Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Revert merge strategy to targeted git checkout (works in shallow clones) - Remove roles:all, restore gh-aw pre_activation with write-level checks - Remove author_association from if: (gh-aw handles access gating) - Update fork fallback message to remove stale workflow_dispatch advice Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add suppress_comment input for workflow_dispatch dry-run (evaluate without posting comment) - Add explicit noop guidance so the agent uses it instead of silently exiting - Update posting results section to respect dry-run mode Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PureWeen
changed the title
Prevents silent fork check bypass when gh returns empty/malformed JSON — $null.isFork evaluates to $false in PowerShell, which would let the fork check pass incorrectly. Note: ready_for_review cannot be added to pull_request_target types yet — gh-aw compiler doesn't include it in the allowed type list. Filed as a known gap. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Fix inaccurate claim that agent has 'no ability to access secrets' — COPILOT_TOKEN is present via --env-all, defended by firewall + redaction + threat detection - Add Security Boundaries section with principles from GitHub Security Lab's pwn-request guidance - Add defense layers table documenting what each layer does/doesn't do - Add explicit rules for workflow authors (DO/DON'T) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-authored PRs are created by copilot[bot] which doesn't have write collaborator access. The bots: allowlist lets the pre_activation membership check pass for this known bot actor. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move permission check before fork check — fork PRs from collaborators with write access should be checked out and evaluated. Only block PRs from authors without write access (exit 0, not exit 1 — it's a skip, not an error). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PureWeen
changed the title
| If there is nothing to evaluate (PR has no test files, PR is a docs-only change, etc.), you **must** call the `noop` tool with a message explaining why: | ||
|
|
||
| ```json | ||
| {"noop": {"message": "No action needed: [brief explanation, e.g. 'PR contains no test files']"}} |
There was a problem hiding this comment.
You might want to configure to not generate a no-op run report issue (within the frontmatter).
https://github.github.com/gh-aw/patterns/monitoring/#no-op-run-reports
| ### Key Principles (from [GitHub Security Lab](https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/)) | ||
|
|
||
| 1. **Never execute untrusted PR code with elevated credentials.** The classic "pwn-request" attack is `pull_request_target` + checkout PR + run build scripts with `GITHUB_TOKEN`. The attack surface includes build scripts (`make`, `build.ps1`), package manager hooks (`npm postinstall`, MSBuild targets), and test runners. | ||
|
|
||
| 2. **Treating PR contents as passive data is safe.** Reading, analyzing, or diffing PR code is fine — the danger is *executing* it. Our gh-aw workflows read code for evaluation; they never build or run it. | ||
|
|
||
| 3. **`pull_request_target` grants write permissions and secrets access.** This is by design — the workflow YAML comes from the base branch (trusted). But any step that checks out and runs fork code in this context creates a vulnerability. | ||
|
|
||
| 4. **`pull_request` from forks has no secrets access.** GitHub withholds secrets because the workflow YAML comes from the fork (untrusted). This is the safe default for CI builds on fork PRs. | ||
|
|
||
| 5. **The `workflow_run` pattern separates privilege from code execution.** Build in an unprivileged `pull_request` job → pass artifacts → process in a privileged `workflow_run` job. This is architecturally what gh-aw does: agent runs read-only, `safe_outputs` job has write permissions. |
| pull_request_target: | ||
| types: [opened, synchronize, reopened] |
There was a problem hiding this comment.
I do think this will still lead to the 'Approve and run workflows' button showing up for PRs from untrusted forks. We need to solidify the guidance we give for when to hit that button. I really wish that button navigated into a list of workflows needing approval for the PR with boxes to select which to approve.
| suppress_comment: | ||
| description: 'Dry-run — evaluate but do not post a comment on the PR' |
There was a problem hiding this comment.
Future-proofing: I suggest renaming to suppress_output in case the output changes (to a PR review for example).
| suppress_comment: | |
| description: 'Dry-run — evaluate but do not post a comment on the PR' | |
| suppress_output: | |
| description: 'Dry-run - evaluate but do not post output on the PR' |
| type: boolean | ||
| default: false | ||
| bots: | ||
| - "copilot[bot]" |
There was a problem hiding this comment.
I am not certain what identity ends up getting used here; I experimented and I think it's one of these but not sure which. 😄
- copilot
- copilot[bot]
- app/copilot-swe-agent
- copilot-swe-agent
- copilot-swe-agent[bot]
| if: >- | ||
| (github.event_name == 'pull_request' && github.event.pull_request.draft == false) || | ||
| (github.event_name == 'pull_request_target' && github.event.pull_request.draft == false) || | ||
| github.event_name == 'workflow_dispatch' || | ||
| (github.event_name == 'issue_comment' && | ||
| github.event.issue.pull_request && |
There was a problem hiding this comment.
I always guard against forks as well, preventing the workflow from running on forks except for the workflow_dispatch event. Otherwise, PRs within a fork will result in failing workflow runs (vs. starting the workflow and skipping all jobs).
Simple case that needs adapting to your scenario: if: (!github.event.repository.fork) || github.event_name == 'workflow_dispatch'.
| When triggered via `workflow_dispatch` with `suppress_comment` = `${{ inputs.suppress_comment }}`: | ||
| - If **true**, perform the full evaluation but **do not** post a comment on the PR. Write the evaluation to the workflow log only. This is useful for testing the skill without spamming the PR. | ||
| - If **false** (default), post the comment as normal. |
There was a problem hiding this comment.
These expressions get replaced on their way to the model so this would end up embedding the true or false value into the opening statement. I think you need something more like this (but I recommend validating my understanding here). Note I also reflected my suggested input rename from above.
| When triggered via `workflow_dispatch` with `suppress_comment` = `${{ inputs.suppress_comment }}`: | |
| - If **true**, perform the full evaluation but **do not** post a comment on the PR. Write the evaluation to the workflow log only. This is useful for testing the skill without spamming the PR. | |
| - If **false** (default), post the comment as normal. | |
| When triggered via `workflow_dispatch`, the `suppress_output` input controls behavior. | |
| - If `${{ inputs.suppress_output }}` == **true**, perform the full evaluation but **do not** post a comment on the PR. Write the evaluation to the workflow log only. This is useful for testing the skill without spamming the PR. | |
| - If `${{ inputs.suppress_output }}` == **false** (default), post the comment as normal. |
| ## Posting Results | ||
|
|
||
| Call `add_comment` with `item_number` set to the PR number. Wrap the report in a collapsible `<details>` block: | ||
| If dry-run mode is active (`suppress_comment` is true), log the evaluation report to stdout and stop — do **not** call `add_comment`. |
There was a problem hiding this comment.
| If dry-run mode is active (`suppress_comment` is true), log the evaluation report to stdout and stop — do **not** call `add_comment`. | |
| If dry-run mode is active (`suppress_output` is true), log the evaluation report to stdout and stop — do **not** call `add_comment`. |
Description
Overhauls the
copilot-evaluate-testsgh-aw workflow for better security, fork support, and Copilot bot compatibility.Changes
pull_requesttopull_request_target— runs workflow YAML from base branch (trusted), eliminates "Approve & Run" friction for collaboratorscopilot[bot]tobots:allowlist — Copilot-authored PRs auto-trigger evaluation without needing write collaborator statussuppress_commentinput) — evaluate without posting comments, useful for testingnooptool guidance — agent callsnoopwhen no action is needed instead of silently exitingCheckout-GhAwPr.ps1— null guard on$PrInfo, allow fork PRs from write-access authorsTrigger Behavior
pull_request_targetsrc/**/tests/**copilot[bot]; blocked for external contributors viapre_activationrole checkissue_comment/evaluate-testscomment on a PRworkflow_dispatchAccess Matrix
pull_request_target/evaluate-testscommentworkflow_dispatchbots:allowlist)pre_activationblockspre_activationblockspre_activationblockspre_activationblocksSecurity Model
Based on GitHub Security Lab guidance:
contents: read, issues: read, pull-requests: read) ✅safe_outputsjob (not the agent) ✅permissions: {}at workflow level — no ambient write access ✅max: 1comment via safe-outputs ✅Checkout-GhAwPr.ps1restores.github/skills/and.github/instructions/from base branch forworkflow_dispatch✅Security Docs Update
Updated
.github/instructions/gh-aw-workflows.instructions.mdwith:COPILOT_TOKENpresent in agent env via--env-all, defended by firewall + redaction + threat detection)Known Limitations
ready_for_reviewevent type not supported by gh-aw compiler forpull_request_target— draft→ready transitions without a new push won't trigger the workflowcheckout_pr_branch.cjsoverwrites.github/skills/forpull_request_target/issue_commenttriggers — accepted residual risk (agent sandboxed, output limited)